Review UI and suggest improvements#88
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the frontend UI/UX for the natural deduction proof app, focusing on improved layout consistency and better accessibility semantics across key components.
Changes:
- Improved overall app layout (toolbar + main area) and added empty-state messaging when no proof is loaded.
- Enhanced accessibility and usability in the menu and modal (ARIA attributes, inline errors instead of
alert(), loading/disabled states, keyboard handling). - Updated proof viewer/table markup and styling for readability and screen-reader navigation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/App.tsx | Adds toolbar/main layout and an empty-proof placeholder when no proof is loaded. |
| frontend/src/App.css | Styles toolbar, focus rings, and empty-proof placeholder. |
| frontend/src/components/Header.tsx | Wraps title in a styled <header> and updates copy. |
| frontend/src/components/Header.css | Adds header banner styling. |
| frontend/src/components/menu/Menu.tsx | Adds empty-state, inline error handling, loading state, and updates labels/copy. |
| frontend/src/components/menu/Menu.css | Adds focus styles, empty-state styling, disabled/button styles, and inline error styling. |
| frontend/src/components/modal/NewProofModal.tsx | Adds dialog ARIA, escape/backdrop close, premise removal UI, focus-on-open, and submit validation. |
| frontend/src/components/modal/NewProofModal.css | Refreshes modal styling and adds premise-row/remove button styling + focus states. |
| frontend/src/components/proof/ProofViewer.tsx | Adds table ARIA improvements and moves divider styling to CSS classes. |
| frontend/src/components/Expressions.css | Reduces proof font sizes, adds hover styling, and introduces wrapper/divider styles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -108,31 +114,57 @@ const Menu: FC<MenuProps> = ({ logic, onColorChange, setProof, proof }) => { | |||
| }; | |||
|
|
|||
| applyAction(logic, proof, actionDto, (response: ApplyActionResponse) => { | |||
| setIsLoading(false); | |||
| if (response.success) { | |||
| setProof(response.proof); | |||
| glowingColors.forEach((color) => onColorChange(color,-1)); | |||
| } else { | |||
| console.error('Action failed:', response.message); | |||
| alert(`Action failed: ${response.message}`); | |||
| setErrorMessage(response.message || 'Action could not be applied.'); | |||
| } | |||
| }); | |||
There was a problem hiding this comment.
isLoading is set to true before calling applyAction, but it’s only reset inside the success callback. If the underlying fetch rejects (see frontend/src/service/actions.ts where .catch(...) only logs and never invokes the consumer), the callback won’t run and the UI will stay stuck in the loading/disabled state. Consider handling promise rejections here (e.g., try/finally with await applyAction(...) or adding an error callback / making applyAction always invoke the consumer) so setIsLoading(false) runs on all paths and an error message is surfaced.
| <label htmlFor="action-select" className="menu-label">Select Inference Rule:</label> | ||
| <select | ||
| id="action-select" | ||
| className="menu-select" | ||
| value={selectedAction} | ||
| onChange={handleActionChange} | ||
| aria-label="Select inference rule" | ||
| > | ||
| <option value="">-- Choose a rule --</option> | ||
| {actions.map((action) => ( | ||
| <option key={action.name} value={action.name}> | ||
| {action.name} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| {selectedInputs} | ||
| {errorMessage && ( | ||
| <p className="menu-error" role="alert" aria-live="assertive"> | ||
| {errorMessage} | ||
| </p> | ||
| )} | ||
| <button | ||
| className="menu-button" | ||
| onClick={processAction} | ||
| disabled={selectedAction === '' || isLoading} | ||
| aria-disabled={selectedAction === '' || isLoading} | ||
| > | ||
| {isLoading ? 'Applying…' : 'Apply Rule'} | ||
| </button> |
There was a problem hiding this comment.
This component’s behavior/labels changed (e.g., option text "-- Choose a rule --", button label "Apply Rule", removed alert() in favor of inline role="alert", and different GlowingInput labels). There are existing Jest/RTL tests for Menu that assert the old strings and window.alert usage, which will now fail. Please update the Menu tests to reflect the new UI and error-handling behavior.
| const hasProof = proof.goal !== ''; | ||
|
|
||
| return ( | ||
| <div className="App"> | ||
| <Header /> | ||
| <button className="new-proof-btn" onClick={handleOpenModal}> | ||
| New Proof | ||
| </button> | ||
| <Menu logic={LOGIC} proof={proof} setProof={setProof} onColorChange={onColorChange} /> | ||
| <Proof proof={proof} coloring={colorMapping} /> | ||
|
|
||
| <div className="app-toolbar"> | ||
| <button | ||
| className="new-proof-btn" | ||
| onClick={handleOpenModal} | ||
| aria-label="Start a new proof" | ||
| > | ||
| New Proof | ||
| </button> | ||
| </div> | ||
| <main className="app-main"> | ||
| <Menu logic={LOGIC} proof={proof} setProof={setProof} onColorChange={onColorChange} /> | ||
| {hasProof ? ( | ||
| <Proof proof={proof} coloring={colorMapping} /> | ||
| ) : ( | ||
| <div className="empty-proof-state" role="status"> | ||
| <p>No proof loaded. Click <strong>New Proof</strong> to begin.</p> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
hasProof is currently derived from proof.goal !== '', while the Menu’s empty-state uses proof.steps.length === 0 && !proof.goal. If a proof ever has steps but an empty goal (e.g., from other flows/tests/service responses), App will show the empty-proof placeholder while Menu shows the rule-application UI, which is inconsistent and could hide an active proof. Consider basing both empty/loaded checks on the same predicate (e.g., proof.goal !== '' || proof.steps.length > 0).
| <h2 id="new-proof-modal-title">New Proof</h2> | ||
| <div className="modal-body"> | ||
| <label>Premises:</label> | ||
| <label className="modal-section-label">Premises:</label> | ||
| {premises.map((premise, index) => ( | ||
| <input | ||
| key={premise+index} | ||
| type="text" | ||
| value={premise} | ||
| onChange={(e) => handlePremiseChange(index, e.target.value)} | ||
| placeholder="Enter a premise" | ||
| /> | ||
| <div key={index} className="premise-row"> | ||
| <input | ||
| ref={index === 0 ? firstInputRef : undefined} | ||
| id={`premise-${index}`} | ||
| type="text" | ||
| value={premise} | ||
| onChange={(e) => handlePremiseChange(index, e.target.value)} | ||
| placeholder={`Premise ${index + 1}`} | ||
| aria-label={`Premise ${index + 1}`} | ||
| /> | ||
| {premises.length > 1 && ( | ||
| <button | ||
| className="remove-premise-btn" | ||
| onClick={() => handleRemovePremise(index)} | ||
| aria-label={`Remove premise ${index + 1}`} | ||
| title="Remove" | ||
| > | ||
| × | ||
| </button> | ||
| )} | ||
| </div> | ||
| ))} | ||
| <button className="add-premise-btn" onClick={handleAddPremise}> | ||
| + Add Premise | ||
| </button> | ||
|
|
||
| <label>Goal:</label> | ||
| <label htmlFor="modal-goal" className="modal-section-label">Goal:</label> | ||
| <input | ||
| id="modal-goal" | ||
| type="text" | ||
| value={goal} | ||
| onChange={(e) => handleGoalChange(e.target.value)} | ||
| placeholder="Enter the goal" | ||
| placeholder="Enter the goal expression" | ||
| /> | ||
| </div> | ||
| <div className="modal-footer"> | ||
| <button className="submit-btn" onClick={handleSubmit}> | ||
| Submit | ||
| <button | ||
| className="submit-btn" | ||
| onClick={handleSubmit} | ||
| disabled={goal.trim() === ''} | ||
| > | ||
| Start Proof | ||
| </button> | ||
| <button className="close-btn" onClick={onClose}> | ||
| Close | ||
| Cancel | ||
| </button> |
There was a problem hiding this comment.
There are existing Jest/RTL tests for NewProofModal that assert the old placeholders ("Enter a premise" / "Enter the goal") and button labels ("Submit" / "Close"). With the new placeholders, "Start Proof"/"Cancel" labels, and the submit button now disabled until a goal is entered, those tests will fail unless updated to match the new UI and validation behavior.
|
|
Summary
Reviewed the frontend UI and applied concrete improvements across usability, accessibility, and visual consistency:
<h1>with a proper<header>banner and readable title.position: absoluteinto a flexbox toolbar; added empty-state message when no proof is loaded.alert()on failure with inlinerole="alert"error; added loading state ("Applying…"); relabeled button to "Apply Rule"; added keyboard focus ring styles.role="dialog",aria-modal,aria-labelledby; Escape closes modal; backdrop click closes; per-premise remove button; Submit disabled until goal is entered.aria-labelto table andscope="col"to<th>for screen reader navigation; extracted inline<hr>styles to CSS.3remto1.4rem; added row hover effect.Opened by multi-repo-tasks via Claude Code.